Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Rebased) Pr/behind proxy middleware #1660

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

xsawyerx
Copy link
Member

@xsawyerx xsawyerx commented Mar 31, 2022

This is a rebased branch of #590. :)

It's only for review against the original PR. Please do not merge it.

Uses existing plack middlewares (ReverseProxy and ReverseProxyPath) to
handle all request header modification when operating behing a proxy. Allows
for variants (eg HTTP_X_FORWARDED_PROTOCOL) supported by Dancer(2) but not
from those existing ReverseProxy middleware.

Special cases REQUEST_BASE header to work with ReverseProxyPath so proxies
from/to non-root paths "just work"(tm). (Alternate to #571.)

As a middleware, devs get more flexability as to where to apply it; they can
use Plack::Builder to wrap this around their app as well as further path/header
altering middleware. This could be released as a seperate package; its not
Dancer2 specific.
  set behind_proxy => 1;

We now 'use' the middleware directly rather than letting Plack::Builder
load it for us. May allow earlier version of Plack to be used as a dependency.
@xsawyerx xsawyerx requested review from cromedome and veryrusty March 31, 2022 15:38
@xsawyerx xsawyerx changed the title Pr/behind proxy middleware rebased (Rebased) Pr/behind proxy middleware Apr 1, 2022
Copy link
Contributor

@cromedome cromedome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like how this came together. We removed a test in t/request.t and we don't have any tests for lib/Dancer2/Middleware/BehindProxy.pm. Asking here so we can capture this for our future selves: do we not need to test this going forward because of the changes, or should we re-implement the test we removed in another fashion? That's about my only concern moving forward with this.

@@ -0,0 +1,81 @@
use strict;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this test file is supposed to be part of this PR. It's causing the Appveyor tests to fail.

@xsawyerx xsawyerx force-pushed the pr/behind_proxy_middleware-rebased branch from 9d4d838 to 2faca00 Compare April 1, 2022 17:08
@@ -0,0 +1,58 @@
package Dancer2::Middleware::BehindProxy;
# ABSTRACT: Support Dancer2 apps when operating behing a reverse proxy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: behind

@veryrusty
Copy link
Member

I still think this is a better way to handle the psgi env munging needed (compared with including it as part of our Request class).

It's taken some time, but I concur with the comments on the original PR about the namespace; something like Plack::Middleware::Dancer2BehindProxy would be better (and hidden from pause?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants